Fix error after remote tidy timeout #4465
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MetRonnie
commented
Oct 12, 2021
| # Terminate any remaining commands | ||
| for platform_n, (cmd, proc) in procs.items(): |
There was a problem hiding this comment.
This was the offending line
MetRonnie
force-pushed
the
remote-tidy-fix
branch
from
4818895 to
b40a558
Compare
Nice, kudos if that works out 🥇 |
| self.bad_hosts.add(host) | ||
| while queue and time() < timeout: | ||
| item = queue.popleft() | ||
| if item.proc.poll() is None: # proc still running |
There was a problem hiding this comment.
Could use proc.wait(timeout=10) to avoid this polling loop and the subsequent termination loop e.g:
for proc in procs:
try:
if proc.wait(10):
...
else:
...
except subprocess.TimeoutExpired:
...There was a problem hiding this comment.
I think that would negate the concurrency in the way the queue is currently handled?
There was a problem hiding this comment.
The processes would still be concurrent, however, the result retrieval would be sequential:
from subprocess import *
from time import time
procs = [
Popen(['sleep', '2'])
for _ in range(5)
]
start = time()
for proc in procs:
proc.wait()
print(f'{time() - start}')$ python test.py
2.007575035095215There was a problem hiding this comment.
I see, but considering that retries for SSH 255 failures are queued after result retrieval, it would still slow it down in those cases, I'd have thought
There was a problem hiding this comment.
Ah, damn, adding to the list whilst processing it makes a mess of things.
datamel
reviewed
Oct 13, 2021
|
Number of failed tests (before retry) now a lot less. |
datamel
reviewed
Oct 13, 2021
tests/functional/intelligent-host-selection/01-periodic-clear-badhosts.t
Outdated
Show resolved
Hide resolved
Avoid maxing out CPU!
MetRonnie
force-pushed
the
remote-tidy-fix
branch
from
3a7321b to
19d7e59
Compare
oliver-sanders
added
bug
oliver-sanders
approved these changes
Oct 14, 2021
MetRonnie
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a small change with no associated Issue.
Fix a
ValueErrorthat could occur during remote tidy during shutdown:I think that might have been why
_remote_background_indep_pollwas so flaky on GH Actions.In the process I refactored
TaskRemoteMgr.remote_tidy()to use a queue of processes instead of a dict, much like how remote clean works.Requirements check-list
CONTRIBUTING.mdand added my name as a Code Contributor.setup.pyandconda-environment.yml.